Use effects for indirect call expressions#8625
Conversation
690ee8d to
75481a7
Compare
189d543 to
2630eff
Compare
75481a7 to
36a4fb5
Compare
|
Seems like JJ + Github don't play well when I'm on a branch based on another branch that had a merge commit. Will fix this after merging the other branch. |
When running in --closed-world, compute effects for indirect calls by unioning the effects of all potential functions of that type. In --closed-world, we assume that all references originate in our module, so the only possible functions that we don't know about are imports. Previously [we gave up on effects analysis](https://github.com/WebAssembly/binaryen/blob/29b2d42e8a748fbe1095696d58a52b7bf83e2253/src/passes/GlobalEffects.cpp#L83-L87) for indirect calls. Yields a very small byte count reduction in calcworker (3799354 - 3799297 = 57 bytes). Also shows no significant difference in Binaryen runtime: (0.1346069 -> 0.13375045 = <1% improvement, probably within noise). We expect more benefits after we're able to share indirect call effects with other passes, since currently they're only seen one layer up for callers of functions that indirectly call functions (see the newly-added tests for examples). Followups: * Share effect information per type with other passes besides just via Function::effects (#8625) * Exclude functions that don't have an address (i.e. functions that aren't the target of ref.func) from effect analysis () * Compute effects more precisely for exact + nullable/non-nullable references Part of #8615.
36a4fb5 to
e65a243
Compare
30a31e1 to
60665f3
Compare
60665f3 to
2156d99
Compare
| } | ||
| } | ||
|
|
||
| if (effects.throws_ && (parent.tryDepth > 0 || curr->isReturn)) { |
There was a problem hiding this comment.
When do we use effect.throws() and when to use effect.throws_?
(Should we use throws() in all cases?)
There was a problem hiding this comment.
Good catch! throws() is throws_ || !delegateTargets.empty(). If we're delegated to and we catch, then it would be correct for throws() to be false, but it would remain true in our case because we can only clear throws_ (it wouldn't be right to just clear delegateTargets because it looks like it's used for other things too, even if the delegated exception gets caught). So we're overly pessimistic in this case.
This way preserves the existing logic (link) so I'd prefer to keep it as-is and fix it in a future PR. Let me try to repro a case where our effects are overly pessimistic and file an issue.
There was a problem hiding this comment.
Maybe my memory of how this delegateTargets is working is patchy (even though it's probably me who added this..).
Good catch!
throws()isthrows_ || !delegateTargets.empty(). If we're delegated to and we catch, then it would be correct forthrows()to be false
How can we at the same time be delegated to and catch? If we are a target of a delegate, that means we are try-delegate, not catch, no?
(it wouldn't be right to just clear
delegateTargetsbecause it looks like it's used for other things too, even if the delegated exception gets caught).
What are the other things it used for?
Anyway, keeping the existing logic as is in this PR and follow-up later sounds fine.
There was a problem hiding this comment.
How can we at the same time be delegated to and catch?
I think this is the typical case for try-delegate? e.g. here I think the $outer_handler block is a delegateTarget and catches, so it should have no throw effect:
(func
try $outer_handler
try
call $potentially_throwing_function
delegate $outer_handler ;; Forwards the exception to the outer try block
catch
call $handle_error
end
)OTOH we can also delegate to a block that doesn't catch at all, in which case that block needs to have a throw effect, e.g. here the anonymous block for the function body should have a throws effect:
(func
try
try
call $some_throwing_function
delegate 1 ;; Delegates to the caller, bypassing the outer try/catch
catch
;; This catch block is completely ignored
end
)(I just read about this feature now FWIW, so I could be wrong)
What are the other things it used for?
I thought it was used in another pass but after looking again it's just shadowing the name. Still, just clearing delegateTargets seems wrong; even if an expression catches exceptions, it seems misleading to say that it wasn't delegated to.
There was a problem hiding this comment.
Ah right, your understanding is correct; I was confused. We handle the "catching" effect in catch_all (because tryDepth> 0) but still conservative because we don't check tag matches for catch, also we don't check whether the try-delegate body itself throws or not. Given that catch_all greatly outnumbers catch at least in C++ (due to destructors), I doubt fixing the former would be a meaningful win, but maybe the latter can be a good missed opportunity. Not sure.
Lines 565 to 586 in 36b7033
Anyway I'm not still sure in what context "clearing delegateTargets" popped up, but anyway, I don't see any reason we should do it when handling calls.
| // TODO: Use Type instead of HeapType to account for nullability and | ||
| // exactness. | ||
| std::unordered_map<HeapType, std::shared_ptr<const EffectAnalyzer>> | ||
| typeEffects; |
There was a problem hiding this comment.
How is this managed? Who is responsible for updating it? Please document this here.
| it != parent.module.typeEffects.end() && it->second) { | ||
| bodyEffects = it->second.get(); | ||
| } | ||
| populateEffectsForCall(curr, bodyEffects); |
There was a problem hiding this comment.
Perhaps processCall or addCallEffects, which are shorter?
| // GlobalEffects. Note that calls may have other effects that aren't | ||
| // captured by the function body of the target (e.g. a call_ref may trap on | ||
| // null refs). | ||
| template<typename CallType> |
There was a problem hiding this comment.
| template<typename CallType> | |
| template<typename T> |
I believe this is the pattern we use in general.
| using Ts::operator()...; | ||
| }; | ||
|
|
||
| template<typename T> using NullablePtr = T; |
| template<typename CallType> | ||
| void populateFunctionBodyEffects(const CallType* curr, | ||
| const EffectAnalyzer& funcEffects) { | ||
| if (curr->isReturn) { |
There was a problem hiding this comment.
populateFunctionBodyEffects and populateEffectsForCall overlap here, I think? This is adding an effect from the call, not the body. Maybe I don't understand the difference between the functions - perhaps document them more?
| parent.trap = true; | ||
| return; | ||
| } | ||
| if (table->type.isNullable()) { |
There was a problem hiding this comment.
Is there a reason we check only this but call trapOnNull on visitCallRef? Should we call trapOnNull on both cases?
| parent.throws_ = true; | ||
| const EffectAnalyzer* bodyEffects = nullptr; | ||
| if (auto it = parent.module.typeEffects.find(curr->heapType); | ||
| it != parent.module.typeEffects.end() && it->second) { |
There was a problem hiding this comment.
Is there a case it->second is null?
Part of #8615. After #8609, we compute effects for indirect call expressions, but only reflect this in the call-site via the effects of the
Functionthat contains the indirect call. That let us reason about effects only one layer of indirection away, for example in the following module:If we know that an indirect call to $t can't possibly have any effects (e.g. its only potential target is a nop), we'd be able to optimize away
(call $a)but not the(call_ref)itself, since the effects only got stored in the effects of$a.This PR lets us reason about indirect call effects at the expression level within function bodies by adding a map from HeapType to effects
typeEffectsinwasm::Module. As a result we can completely optimize out thecall_refin the above example.Drive-by fixes:
Correctly setWill follow up in return_call with call.without.effects optimizes incorrectly #8693.branchesOutforreturn_calloncall.without.effects. Previously this would not have abranchesOuteffect which may have allowed incorrect reorderings (we shouldn't move an effectful expression above areturn_callbut we would have allowed this).